Skip to content

fix(updex): refuse legacy sysext runners instead of falling back to SysextDir - #405

Draft
bketelsen wants to merge 1 commit into
mainfrom
fix/sysext-link-dir-capture
Draft

fix(updex): refuse legacy sysext runners instead of falling back to SysextDir#405
bketelsen wants to merge 1 commit into
mainfrom
fix/sysext-link-dir-capture

Conversation

@bketelsen

Copy link
Copy Markdown
Contributor

Summary

ClientConfig.SysextRunner was the one hole in the ADR-0011
capture-at-construction invariant. A runner implementing only the original
four-method sysext.SysextRunner was linked through its pathless
LinkToSysext, and Client.sysextLinkDirForRunner() returned the mutable
package-global sysext.SysextDir — read at call time, long after NewClient.
A client configured with RuntimePaths.SysextLinkDir therefore linked
(and snapshotted its catalog rollback state) somewhere else entirely, while
docs/specs/sdk-api.md asserted that "mutating ... sysext.SysextDir cannot
redirect the client".

This change removes the fallback rather than papering over it:

  • Client.linkToSysext links only through sysext.PathSysextRunner, with
    c.paths.sysextLinkDir. sysextLinkDirForRunner is gone; the catalog
    rollback snapshot (updex/catalog.go) uses the captured directory directly.
  • New exported sentinel updex.ErrLegacySysextRunner (testable with
    errors.Is). Every non-dry-run operation that may end in a link checks
    requireLinkableRunner() before it mutates anything: CatalogAdd
    before writing a definition, EnableFeature with Now before writing the
    drop-in, installTransfer before removing a legacy CurrentSymlink or
    downloading. A legacy runner therefore fails cleanly instead of part-way
    through an install into the wrong directory.
  • Dry runs never link, so they remain available to any runner.
  • The SysextRunner interface is untouched: existing implementations still
    compile, and adding LinkToSysextAt is what makes one usable for a real
    install.
  • sysext.MockRunner implements PathSysextRunner (compile-time assertion
    included) and records the directory it was handed in LinkToSysextAtDir;
    LinkToSysextCalled is still set by either entry point, so existing
    assertions are unaffected.
  • docs/specs/sdk-api.md and docs/design/overview.md now state the legacy
    behavior precisely instead of overstating client isolation.

Resolves the Snowcat architecture-gap item
28c975eb-f265-4597-aefb-d4c66080df4a.

The only remaining sysext.SysextDir read in non-test updex code is
resolveRuntimePaths (updex/updex.go:142) — the construction-time capture
itself.

Checks

  • make fmt — code is formatted (gofmt -l clean; make ci's gofmt
    stage passes)
  • make ci — tidy, vet, gofmt, lint (.golangci.yml), unit tests, the
    80.0% coverage floor (make test-coverage-check then
    make coverage-check), race tests, linux amd64/arm64 builds
==> verify: go.mod is tidy
==> verify: go vet
==> verify: gofmt
==> lint (golangci-lint 2.13.1)
==> unit tests with coverage
==> coverage floor
==> end-to-end tests
==> race detector
==> cross-architecture build
==> CI gate passed
  • CLI/e2e changes: covered by make ci's ./tests/e2e/... stage (green);
    no CLI surface changed by this PR
  • New or changed behavior has focused tests, including failure paths

New tests in updex/install_link_test.go, all against a client constructed
with an instance SysextLinkDir whose package global is redirected to a
different temp dir after construction:

  • TestUpdateFeatures_LegacyRunnerRefusedBeforeAnyMutation — the legacy
    runner's LinkToSysext is never called, the redirected global dir and the
    captured link dir are both empty, and the target dir still holds only the
    staged image.
  • TestEnableFeature_Now_LegacyRunnerRefusedBeforeDropIn
    errors.Is(err, ErrLegacySysextRunner) and no drop-in was written.
  • TestEnableFeature_DryRunNowStillWorksWithLegacyRunner — the dry-run path
    is unaffected.
  • TestUpdateFeatures_PathRunnerReceivesCapturedLinkDir — a
    PathSysextRunner is handed the captured dir, never the moved global.

Falsification: with the guards removed and the old fallback restored, the two
refusal tests fail (expected UpdateFeatures to refuse a legacy sysext runner, EnableFeature error = <nil>, want ErrLegacySysextRunner); with the
fix in place all seven link tests pass. The pre-existing
DefaultRunner-backed link tests (RestoresSysextLinkForCurrentImage,
LeavesCorrectSysextLinkAlone, EnableFeature_Now_RestoresSysextLinkForCurrentImage)
are unchanged and still green.

Risk classification

  • Tier 1: Low
  • Tier 2: Moderate
  • Tier 3: High

Rationale:

  • The diff touches sysext/**, a review-required protected boundary in
    policies/agent-governance.json (installation-and-update) with a
    minimum_risk_tier of high, and it changes a privileged filesystem-write
    path: which directory a client is allowed to create sysext symlinks in.
    It is also a behavior break for any out-of-tree SysextRunner that lacks
    LinkToSysextAt — such a runner now returns ErrLegacySysextRunner
    instead of silently linking into /var/lib/extensions. That refusal is the
    point (the old behavior wrote outside the client's declared directory), but
    it is a compatibility change and takes the highest applicable tier.

Docs housekeeping

  • README.md, docs/design/overview.md, docs/specs/* updated for
    behavior changes; AGENTS.md for convention/workflow changes —
    docs/specs/sdk-api.md (the NewClient isolation paragraph and the
    sysext package reference) and docs/design/overview.md (testing
    patterns) updated; no README.md or AGENTS.md change is implied.
  • New docs started from their category's TEMPLATE.md and indexed in
    docs/README.md — no new docs; existing docs edited in place.
  • New significant decision recorded as an ADR first, in this PR — not
    applicable: this enforces the existing ADR-0011 invariant rather than
    deciding anything new.
  • Conformance aliases (ADR-0012) untouched — canonical targets edited
    instead.

Verification

  • node scripts/check-docs.mjs green
ok   docs_index_coverage: 1.000 (required 1)
ok   link_integrity: 1.000 (required 1)
ok   symlink_resolution: 1.000 (required 1)
checked: 20 docs, 277 links, 9 symlinks

make test-docs-check also green (test-check-docs: all assertions passed).

…ysextDir

An injected SysextRunner that predates PathSysextRunner could not be told
which directory to link into, so the SDK linked through its pathless
LinkToSysext and read the mutable package-global sysext.SysextDir at call
time. That silently ignored RuntimePaths.SysextLinkDir and broke the ADR-0011
capture-at-construction invariant the SDK spec claims.

The client now links only through sysext.PathSysextRunner with the directory
NewClient captured, and every non-dry-run operation that may link refuses a
legacy runner with the new updex.ErrLegacySysextRunner before it mutates
anything: CatalogAdd before writing a definition, EnableFeature --now before
writing the drop-in, installTransfer before removing a legacy symlink or
downloading. The catalog rollback snapshot uses the captured directory too.

The SysextRunner interface is unchanged, so existing implementations still
compile. sysext.MockRunner gains LinkToSysextAt (recording the directory in
LinkToSysextAtDir; LinkToSysextCalled is still set by either entry point) and
a compile-time PathSysextRunner assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UigLsmd17TVDuaaSN4yfdU
@github-actions github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code labels Aug 28, 2026
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
sysext/mock_runner.go 0.00% 4 Missing ⚠️
updex/catalog.go 50.00% 1 Missing and 1 partial ⚠️
updex/install.go 87.50% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants